Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added the onLongTapDown event #1587

Merged
merged 22 commits into from
May 4, 2022
Merged

Conversation

st-pasha
Copy link
Contributor

@st-pasha st-pasha commented Apr 28, 2022

Description

  • Event onLongTapDown added to mixins:
    • Tappable
    • HasTappables
    • MultiTouchTapDetector (it existed here before, but was never invoked, so it was essentially broken)
  • Added documentation to all aforementioned mixins.
  • Due to the fact that HasTappables and MultiTouchTapDetector are very similar in their structure, created a common interface MultiTapListener that both of them implement.
    • This common interface allows removing code duplication at the GameWidget level in file gestures.dart.

Checklist

  • The title of my PR starts with a Conventional Commit prefix (fix:, feat:, docs: etc).
  • I have read the Contributor Guide and followed the process outlined for submitting PRs.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • [-] I have updated/added relevant examples in examples.

Breaking Change

  • [-] Yes, this is a breaking change.
  • No, this is not a breaking change.

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself looks good! But I don't think the directory structure should change in this PR.

Comment on lines +9 to +12
/// Instead of implementing this class directly consider using one of the
/// prebuilt mixins:
/// - [HasTappables] for a `FlameGame`
/// - [MultiTouchTapDetector] for a custom `Game`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I am misremembering, lmk if this comment makes sense, but can't you use MultiTouchTapDetector on FlameGames if you are not using tappables and want to listen to multi-touch events?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since FlameGame is a Game, you can absolutely use MultiTouchTapDetector on it. The HasTappables mixing is just a more concrete implementation, which ensures the event propagates down into the component tree.

Copy link
Member

@luanpotter luanpotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@spydon spydon enabled auto-merge (squash) May 4, 2022 15:57
@spydon spydon merged commit ed302d8 into flame-engine:main May 4, 2022
@st-pasha st-pasha deleted the ps/merge-events branch May 4, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants